Honor permissions of new link share via OCS API #35213
Conversation
szaimen
requested review from
juliusknorr,
come-nc,
nickvergessen,
a team and
ArtificialOwl
and removed request for
a team
|
It would be still nice to preserve the default of read only permissions for the share links, maybe by moving that to the first condition in https://github.com/nextcloud/server/pull/35213/files#diff-dbbe017dd357504abc442a6f1d0305166520ebf80353f42814b3f879a3e241bcR481 What do you think? |
|
+1 for keeping defaults - although it seems wrong to willfully ignore the setting. Changed nonetheless. |
|
The 3 failing tests all explicitly sets PERMISSION_ALL but expects the share to only have read permissions. Fixed two of them to expect PERMISSION_ALL and one to not set permissions at all and expect READ|SHARE. |
The API currently overrides the supplied permissions with "read only" when a file is shared via link. It allows to update the permissions later, however. This keeps the default to "read only" but honors the permissions supplied by API call if any. Signed-off-by: Jan-Philipp Litza <jpl@plutex.de>
|
The amount of tests that cement the old behavior is staggering. After fixing 4 unit tests, now acceptance-app-files-sharing is failing in a way I don't quite understand yet. Apparently a readonly user (!) share can now be re-shared readwrite. I don't yet see any connection to my patch, but it's close enough to probably be caused by it. |
|
The acceptance tests can be randomly failing from time to time. |
|
I've restarted the drone job to double check |
|
Indeed, now the a acceptance-app-files-sharing tests all succeed (but acceptance-app-theming still fails, though probably unrelatedly, and samba now failed, which it previously didn't). |
|
Yes, seems all unrelated. |
|
Sooo... what is the process to get a second review on this? |
There was a problem hiding this comment.
Looks good.
@skjnldsv you added a TODO about it in the front-end, but I don't understand what it is referring too. Should we do something there too ? As this PR seems to "fix the createShare method".
server/apps/files_sharing/src/components/SharingEntryLink.vue
Lines 697 to 701 in 4f68d13
Backend permissions are always tricky for sharing. |
|
Without having looked into the frontend code, I don't see how this PR could pose a problem. It preserves the default, and the frontend always first creates the share link without specifying permissions, and only allows setting permissions later on. |
juliusknorr
added
4. to release
|
Failures unrelated |
|
Thanks for the pull request @jplitza 🎉 |
The API currently overrides the supplied permissions with "read only" when a file is shared via link. It allows to update the permissions later, however.
Fixes #32611